Feature/deseng668: Added authoring side nav and bottom nav, modified routing, added authoring context, added base page for banner #2581

jareth-whitney · 2024-08-24T00:32:24Z

Issue #: https://citz-gdx.atlassian.net/browse/DESENG-668

Description of changes:

Feature New authoring content section
- Implemented authoring side nav
- Implemented authoring bottom nav
- Modified routing
- Implemented authoring section context
- Still working on skeletons
- Merged with main
- Merged with Nat's feature branch
- Made some post-merge fixes

User Guide update ticket (if applicable):
N/A

… page for authoring section.

…icting action names, changed some data fetches.

codecov-commenter · 2024-08-24T00:39:18Z

Codecov Report

Attention: Patch coverage is 45.78313% with 45 lines in your changes missing coverage. Please review.

Project coverage is 75.74%. Comparing base (69524c5) to head (33c3cf2).

Files with missing lines	Patch %	Lines
...gement/admin/create/authoring/AuthoringSideNav.tsx	33.89%	38 Missing and 1 partial ⚠️
...nts/engagement/admin/view/AuthoringTabElements.tsx	20.00%	4 Missing ⚠️
.../components/engagement/admin/view/AuthoringTab.tsx	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2581      +/-   ##
==========================================
- Coverage   76.02%   75.74%   -0.28%     
==========================================
  Files         597      603       +6     
  Lines       21652    21811     +159     
  Branches     1770     1811      +41     
==========================================
+ Hits        16460    16521      +61     
- Misses       4936     5028      +92     
- Partials      256      262       +6

Flag	Coverage Δ
metweb	`64.90% <45.78%> (-0.38%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...web/src/components/common/RichTextEditor/index.tsx	`80.00% <ø> (ø)`
...nt/admin/create/authoring/AuthoringNavElements.tsx	`100.00% <100.00%> (ø)`
...et-web/src/components/engagement/listing/index.tsx	`76.12% <100.00%> (ø)`
...eb/src/components/feedback/FeedbackModal/index.tsx	`88.73% <100.00%> (ø)`
met-web/src/components/imageUpload/index.tsx	`100.00% <ø> (ø)`
...eb/src/components/layout/Header/InternalHeader.tsx	`88.39% <100.00%> (+1.01%)`	⬆️
met-web/src/styles/Theme.ts	`100.00% <ø> (ø)`
.../components/engagement/admin/view/AuthoringTab.tsx	`27.27% <50.00%> (ø)`
...nts/engagement/admin/view/AuthoringTabElements.tsx	`20.00% <20.00%> (ø)`
...gement/admin/create/authoring/AuthoringSideNav.tsx	`33.89% <33.89%> (ø)`

... and 2 files with indirect coverage changes

NatSquared

Hi Jareth - thanks for all your work on these components.
I've made some suggestions based on how I think you're trying to leverage these libraries; feel free to reach out if anything is unclear, or if I've missed something in my review.

met-web/src/components/engagement/admin/create/authoring/AuthoringBottomNav.tsx

met-web/src/routes/AuthenticatedRoutes.tsx

met-web/src/styles/Theme.ts

met-web/src/routes/AuthenticatedRoutes.tsx

met-web/src/components/layout/Header/InternalHeader.tsx

…comments.

Baelx

Great work on this Jareth! You've done tons of work here.

I just want to ensure screen reader users are getting the same experience as others. Can you respond to some of my comments about the use of elements and how they help screen readers? Also, regarding the Preview button, I think we should use an icon button instead there.

Baelx · 2024-09-03T22:36:12Z

met-web/src/components/engagement/admin/create/authoring/AuthoringBottomNav.tsx

@@ -141,7 +117,14 @@ const AuthoringBottomNav = (props: AuthoringBottomNavProps) => {
                                marginLeft: 'auto',
                            }}
                        >
-                            <img style={{ paddingRight: '0.3rem' }} src={pagePreview} alt="page preview icon" />
+                            <img


This should be an icon instead of an image. Also for screenreaders, as long as it reads something like "preview," "page preview," or "editing preview" then we should be good as far as labelling goes.

Jess should have been using FontAwesome icons so it should be available to us

I'm not sure how to make it into an icon since this is a custom graphic that Jessica created for the button. I exported it from Figma and included it in our assets. If we have a process for converting images to icons then let me know and I will do it.

I appreciate that you took the trouble of exactly replicating her design. Is it a PNG? We may still be able to use it as an icon then. You could also switch to a fontawesome icon button with one of these icons here, some of which are close enough to hers: https://fontawesome.com/search?q=magnifying&o=r

However you do it, the image tag should be removed here - it'll cause confusion to screen readers. Practically speaking, all they should hear is "button, editing preview" or something like this. Hearing "image" in this context might throw them off.

I looked it up and apparently I can add alt="" and aria-hidden="true" in order to keep it invisible to a reader.

I'm alright with this as a quick fix this time. Could you please just test it once you implement? Ideally with VoiceOver

Baelx · 2024-09-03T22:42:16Z

met-web/src/components/engagement/admin/create/authoring/AuthoringContext.tsx

            },
        );
    };

+    const getSendReportValue = (valueToInterpret: boolean) => {


Is this like a utility function to convert incompatible data to that used in an HTTP request? Do we not have something for this already?

So the data in createSearchParams all needs to be converted to strings. In this particular case, I just needed to convert a boolean value to a string equivalent as 'true' or 'false'. It's quite possible that there is another utility function that does this, I just don't know about it.

Booleans can't be used with truthy checks because they can return false if their value is false, even though I want that conditional to pass on a false value. So I need to check if the value is undefined separately, then give a value that is either 'true' or 'false'.

Yeah I was just curious how this operation for creating an HTTP request gets handled elsewhere and if we could use that. No worries if you want to just stick with this function you've created

Baelx · 2024-09-03T23:06:12Z

met-web/src/components/engagement/admin/create/authoring/AuthoringFeedback.tsx

+                    <EyebrowText style={eyebrowTextStyles}>
+                        Your section heading should be descriptive, short and succinct.
+                    </EyebrowText>
+                    <TextField


Nat once mentioned that our TextField component attempts to auto generate a label. Could you let me know what the label for this text field will be ? Also, will screen reader users understand this field is required too?

If there's no label here then I'd recommend we make the h3 above into one and just style it to match the designs

I just looked and I'm not seeing an auto-generated label for the TextField component in the browser. Perhaps I need to use a specific prop. If it's ok with you, I think this is something that could be addressed in the individual authoring page tickets.

I appreciate that you've already done a lot of work for the authoring flow but I think it'd be best to handle these now. We already have to update the other tickets to describe what's been done in this one and I'm worried about adding another complicating factor like "rework the HTML to better serve screen readers." If we're publishing the HTML then we should do it right from the get-go.

That said, I don't think it'd be too much work to change each of those elements to a label, updating the styling if needed, then associate them with the input fields. It should be the same fix for each of the pages. This would be one approach.

What's important is just that screen readers read out all this information when a user tabs onto a field - however you want to approach that

I made some other changes that will definitely help the way screen readers read the page. I've now tested it with Pericles and the mac OS voice service. The h3s that were form labels have been converted to form labels. I also removed the alt tag on the image in the "Preview" button and added an aria-invisible tag to it.

Baelx · 2024-09-03T23:17:41Z

met-web/src/components/engagement/admin/create/authoring/AuthoringFeedback.tsx

+
+            <Grid sx={{ ...formItemContainerStyles, backgroundColor: colors.surface.gray[10] }} item>
+                <label htmlFor="body_copy">
+                    <MetHeader3 style={metHeader3Styles}>


Even though this resembles a heading in the design, functionally it's more of a label. Same question as above, do screen readers get enough information about this field in order to fill it in?

It would be read as a heading 3 at the moment, but that could definitely be changed if you think that's confusing to screen readers. Firefox doesn't have the best support for screen readers but I am testing the pages briefly with Pericles.

I'd suggest using Mac OS' built-in Voiceover as it's one of the most used screen readers. It's really important that we test our work with a screen reader to ensure those users are having a good experience too

Thanks, I tested with this too.

Baelx · 2024-09-03T23:18:45Z

met-web/src/components/engagement/admin/create/authoring/AuthoringFeedback.tsx

+                        Body Copy
+                        <span style={{ fontWeight: 'normal' }}> (Required)</span>
+                    </MetHeader3>
+                    <EyebrowText style={eyebrowTextStyles}>


We're giving sighted users all this nice information about what goes in this field. Is it easy for screen reader users to perceive this as well?

That's a good question! It's harder to test screen reader compatibility through Firefox without ChromeVox, but I found a plugin called Pericles and tested it with the page. The text was read by the screenreader, so we should be ok.

Thanks for testing that! It's important not only that a screen reader can read it but also that it's clear that this information is associated with a particular form field. When you tabbed your focus to this field, did you hear all this text read out?

The tab just cycles through links, I believe in ChromeVox to cycle through individual items it's something like CTRL+ALT+CMD+DOWN ARROW (or up arrow). The screen reader reads the entire page including this text otherwise.

Baelx · 2024-09-03T23:20:33Z

met-web/src/components/engagement/admin/create/authoring/AuthoringFeedback.tsx

+                    <MetHeader3 style={metHeader3Styles}>
+                        Survey Call to Action Button <span style={{ fontWeight: 'normal' }}>(Optional)</span>
+                    </MetHeader3>
+                    <EyebrowText style={eyebrowTextStyles}>


My understanding of an "eyebrow" is that it's an additional piece of info that goes above the "eye" - in this case the heading or label. I kind of feel this should be a label itself though

I imported this eyebrow text component and used it because it resembled the type in the design, even though the name might not be 100% accurate. I could import it AS something else if you think it's a bit confusing.

It's not necessarily confusing but it's important that we don't just code for looks but that our HTML is semantic and meaningful. In this case, I almost feel like both the metheader3 and eyebrowtext elements should be labels that describe the form field.

I have removed all the MetHeader3 components and replaced them with MetLabel components that are imported as MetBigLabel. This means that they show up as "p" elements in the DOM instead of "h3". Anything that is a real h3 heading is still an h3. I renamed EyebrowText to FormDescriptionText so that it's clear what its use is. This text is all already contained within a label element so it's part of the label, as is the form component itself.

Baelx · 2024-09-03T23:43:43Z

met-web/src/components/engagement/admin/create/authoring/AuthoringTemplate.tsx

+            <SystemMessage status="warning">
+                Under construction - the settings in this section have no effect.
+            </SystemMessage>
+            {'details' === slug && (


Just curious why this markup lives here instead of in AuthoringDetails.jsx. Was it easier to style or something?

It's just about page positioning. For the details page specifically, there is a "content configuration" that appears above the current language h2. We could change that and have the content local to the Details component.

Just looked back at the designs and that makes sense! Thanks for explaining

Baelx · 2024-09-03T23:50:00Z

met-web/src/components/engagement/admin/create/authoring/AuthoringTemplate.tsx

+                    <Await resolve={languages}>
+                        {(languages: Language[]) => (
+                            <AuthoringBottomNav
+                                isDirty={isDirty}


Nice clean code using form context to pass isDirty into the bottom nav ^_^

met-web/src/components/engagement/admin/create/authoring/engagementAuthoringUpdateAction.tsx

sonarcloud · 2024-09-04T23:45:16Z

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Baelx

Thanks for all your work on this! Approved

NatSquared and others added 6 commits August 19, 2024 15:29

Router restructuring for engagement detail tabs

d0273d8

Fixes to links between routes

cb3439e

feature/deseng668: Integrated side nav, bottom nav, context, and base…

9b7a990

… page for authoring section.

Co-authored-by: Nat² <[email protected]>

02911d0

Merge branch 'dev/tab-router-improvements' into feature/deseng668

68c61b0

feature/deseng668: Revised some routing code after merge, fixed confl…

c145283

…icting action names, changed some data fetches.

jareth-whitney requested review from Baelx and NatSquared August 24, 2024 00:32

NatSquared requested changes Aug 26, 2024

View reviewed changes

feature/deseng668: Revised routes and authoring components as per PR …

e60b306

…comments.

NatSquared approved these changes Aug 27, 2024

View reviewed changes

jareth-whitney added 2 commits September 3, 2024 14:39

feature/deseng668: Added authoring template and skeletons.

0702a23

Merge branch 'main' into feature/deseng668

d0cc4f7

Baelx requested changes Sep 3, 2024

View reviewed changes

feature/deseng668: Revised for accessibility, made some merge fixes.

66191a9

NatSquared self-requested a review September 4, 2024 21:16

jareth-whitney added 2 commits September 4, 2024 16:19

feature/deseng668: Added further screen reader support.

8569d78

feature/deseng668: Set image picker back to original design.

33c3cf2

Baelx approved these changes Sep 4, 2024

View reviewed changes

jareth-whitney merged commit 9e5a4f8 into main Sep 4, 2024
6 of 8 checks passed

jareth-whitney deleted the feature/deseng668 branch September 4, 2024 23:52

Feature/deseng668: Added authoring side nav and bottom nav, modified routing, added authoring context, added base page for banner #2581

Feature/deseng668: Added authoring side nav and bottom nav, modified routing, added authoring context, added base page for banner #2581

Conversation

jareth-whitney commented Aug 24, 2024

codecov-commenter commented Aug 24, 2024 • edited Loading

Codecov Report

NatSquared left a comment

Choose a reason for hiding this comment

Baelx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baelx Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jareth-whitney Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 4, 2024

Quality Gate failed

Baelx left a comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 24, 2024 •

edited

Loading

Baelx Sep 4, 2024 •

edited

Loading

jareth-whitney Sep 4, 2024 •

edited

Loading